Skip to content

Enforce secure process execution in CI - #390

Merged
Priyanshu Agrawal (priyanshu92) merged 2 commits into
mainfrom
users/priyanshu92/enforce-secure-process-ci
Aug 11, 2026
Merged

Enforce secure process execution in CI#390
Priyanshu Agrawal (priyanshu92) merged 2 commits into
mainfrom
users/priyanshu92/enforce-secure-process-ci

Conversation

@priyanshu92

@priyanshu92 Priyanshu Agrawal (priyanshu92) commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

  • add a dependency-free, token-aware analyzer for production Power Pages hooks, shared scripts, and skill scripts
  • reject dynamic exec/execSync commands, non-fixed executables, shell: true, and ambiguous shell option shapes with file/line diagnostics
  • prove the pre-fix activation validator is rejected and the argv-based parent fix is accepted
  • migrate two removable version/browser probes to fixed executable plus argv execution
  • run the analyzer and its fixtures in repository metadata validation for every pull request

Analyzer scope and limits

The audit scans production .js, .cjs, and .mjs files under Power Pages hooks/, scripts/, and skill scripts/. Tests, fixtures, generated content, vendor content, dependencies, assets, and references are excluded through explicit path-segment rules.

The implementation uses a conservative tokenizer and balanced-token call reader because this repository has no root parser dependency. It recognizes CommonJS and ESM imports, aliases, direct/namespace/computed/optional calls, multiline calls, templates, regex literals, local options objects, and common shadowing patterns. It is not a general JavaScript parser: unsupported lexical syntax, unresolved executable/options expressions, spreads, computed keys, and ambiguous child-process shapes fail closed for review.

The current production tree has zero audited exceptions. The exact exception schema and stale-exception drift behavior remain covered by synthetic tests.

CI

The metadata workflow now runs on dependent PR bases as well as main and sets POWER_PLATFORM_SKILLS_TELEMETRY_POWER_PAGES_OPTOUT=1 at job level.

Commands:

node --test scripts/tests/validate-secure-process-execution.test.js
node scripts/validate-secure-process-execution.js

Dependency

This PR is stacked on parent PR #386 and must merge after it. Stack metadata is maintained separately.

  • base branch: users/priyanshu92/fix-power-pages-shell-security
  • live parent head: 27c9c6174f19854847235381f52108a767a77660
  • merge-restacked PR head: de8337658ebe5473deefe3b3213a9ad4fdc68ce2

Checks

  • 46 secure-process analyzer tests
  • production audit across 159 files with zero exceptions
  • 86 focused Playwright MCP, canonical-host, activation/export, hook, integration, and Key Vault security tests
  • repository metadata validators
  • full Power Pages script suite: 1,367 tests
  • git diff --check against the live parent

Copilot AI lite review requested due to automatic review settings August 6, 2026 17:22
Copilot AI reviewed Aug 6, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@tyaginidhi

T-Nid (tyaginidhi) commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Excellent analyzer implementation and fixture coverage. I found one high-priority scope issue and one medium test-rigor issue.

Severity: High

  1. scripts/validate-secure-process-execution.js
    • Scan roots are hardcoded to power-pages (plugins/power-pages/{hooks,scripts,skills}), so equivalent process-exec risks in sibling plugins are not gated by CI.

Severity: Medium
2. scripts/tests/validate-secure-process-execution.test.js

  • Repository audit assertion uses result.files.length > 100 instead of a pinned/tight bound, so under-scanning regressions could pass silently.

Suggested fix:

  • Either broaden scan scope across plugins (with explicit exception handling/rollout if needed), or document this as phase 1 with tracked follow-up.
  • Tighten file-count assertion to an exact (or tightly bounded) expected value to catch scan drift.

Copilot AI review requested due to automatic review settings August 11, 2026 07:41
Copilot AI reviewed Aug 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

scripts/validate-secure-process-execution.js:883

  • This else if branch is mis-indented (the body lines align with the outer scope), which makes the control flow and brace structure hard to verify and easy to break during future edits. Re-indent the branch so its contents are clearly nested under the condition.
    scripts/validate-secure-process-execution.js:64
  • The directory walker will still recurse through large non-production trees under plugins/power-pages/skills/**/assets and .../references (they’re not in EXCLUDED_DIRECTORY_NAMES), even though shouldScan() later filters them out. This can add significant CI/runtime overhead and contradicts the stated exclusion of assets/references by path segment rules.
  • Files reviewed: 45/45 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings August 11, 2026 08:07
Copilot AI reviewed Aug 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

scripts/tests/validate-secure-process-execution.test.js:187

  • This assertion makes the test brittle as the repo evolves (file count can legitimately drop below 100, causing unrelated failures). Prefer asserting that the scan includes at least one file from each intended scan root (hooks/scripts/skills scripts).
    scripts/validate-secure-process-execution.js:866
  • The direct-require computed-member branch in findCalls() has misleading indentation (lines inside the else if block are not indented under the opening brace). This makes the nested control flow hard to audit and easy to introduce mistakes in future edits.
  • Files reviewed: 45/45 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

stack merge was automatically disabled August 11, 2026 08:41

Pull Request is not mergeable

stack merge was automatically disabled August 11, 2026 08:43

Pull Request is not mergeable

stack merge was automatically disabled August 11, 2026 08:44

Pull Request is not mergeable

stack merge was automatically disabled August 11, 2026 08:45

Pull Request is not mergeable

stack merge was automatically disabled August 11, 2026 08:55

Pull Request is not mergeable

stack merge was automatically disabled August 11, 2026 08:57

Pull Request is not mergeable

stack merge was automatically disabled August 11, 2026 09:07

Pull Request is not mergeable

Base automatically changed from users/priyanshu92/fix-power-pages-shell-security to main August 11, 2026 09:52
@priyanshu92
Enforce secure process execution
2de0810 
- add a dependency-free, fail-closed child_process analyzer\n- cover safe and unsafe syntax with deterministic fixtures\n- run the audit for every pull request with telemetry disabled\n- migrate removable process probes to fixed executable argv calls\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@priyanshu92
Remove obsolete process audit exception
7625953 
- remove the Playwright shell:true exception after the hardened launcher landed\n- require repository and CLI audits to report zero exceptions\n- retain exact exception schema and stale-drift coverage\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: f349adcb-b52e-4060-95aa-b0f4e13841ac
@priyanshu92
Priyanshu Agrawal (priyanshu92) force-pushed the users/priyanshu92/enforce-secure-process-ci branch from de83376 to 7625953 Compare August 11, 2026 09:52
@priyanshu92
Priyanshu Agrawal (priyanshu92) merged commit 5ebf14d into main Aug 11, 2026
8 checks passed
@priyanshu92
Priyanshu Agrawal (priyanshu92) deleted the users/priyanshu92/enforce-secure-process-ci branch August 11, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@tyaginidhi T-Nid (tyaginidhi) tyaginidhi approved these changes
@amitjoshi438 amitjoshi438 amitjoshi438 approved these changes
@gokulraj-g gokulraj-g gokulraj-g approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@priyanshu92 @tyaginidhi @amitjoshi438 @gokulraj-g